-
Notifications
You must be signed in to change notification settings - Fork 132
[Dynamic Dashboard] Orders card #11486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
Found 1 violations: The PR caused the following dependency changes:expand
-+--- org.wordpress:fluxc:trunk-cfe34975d20df76a2b7b4522dccd2faf53ef0f7c
-| +--- org.wordpress:wellsql:2.0.0
-| | +--- androidx.annotation:annotation:1.2.0 -> 1.7.0 (*)
-| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-| +--- org.wordpress.fluxc:fluxc-annotations:trunk-cfe34975d20df76a2b7b4522dccd2faf53ef0f7c
-| +--- org.greenrobot:eventbus:3.3.1 (*)
-| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| +--- com.android.volley:volley:1.1.1 -> 1.2.0
-| +--- androidx.paging:paging-runtime:2.1.2
-| | +--- androidx.paging:paging-common:2.1.2
-| | | +--- androidx.annotation:annotation:1.0.0 -> 1.7.0 (*)
-| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.6.2 (*)
-| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.6.2 (*)
-| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
-| +--- com.goterl:lazysodium-android:5.0.2
-| +--- net.java.dev.jna:jna:5.5.0
-| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.9.10 (*)
-| +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.20 -> 1.9.22
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.22 (*)
-| +--- androidx.appcompat:appcompat:1.0.2 -> 1.6.1 (*)
-| +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
-| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
-| | \--- androidx.annotation:annotation:1.2.0 -> 1.7.0 (*)
-| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
-| | +--- androidx.annotation:annotation:1.1.0 -> 1.7.0 (*)
-| | +--- com.google.crypto.tink:tink-android:1.5.0
-| | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
-| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
-| | +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
-| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
-| +--- org.apache.commons:commons-text:1.10.0 (*)
-| +--- androidx.room:room-runtime:2.4.2 -> 2.5.2 (*)
-| +--- androidx.room:room-ktx:2.4.2 -> 2.5.2
-| | +--- androidx.room:room-common:2.5.2 (*)
-| | +--- androidx.room:room-runtime:2.5.2 (*)
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
-| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.7.3 (*)
-| +--- com.google.dagger:dagger:2.42 -> 2.50
-| | \--- javax.inject:javax.inject:1
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.7.3 (*)
-| \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.7.3 (*)
++--- org.wordpress:fluxc:3008-f3ff9689a0a998537a624ce9a77b930a341bee48
+| +--- org.wordpress:wellsql:2.0.0
+| | +--- androidx.annotation:annotation:1.2.0 -> 1.7.0 (*)
+| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
+| +--- org.wordpress.fluxc:fluxc-annotations:3008-f3ff9689a0a998537a624ce9a77b930a341bee48
+| +--- org.greenrobot:eventbus:3.3.1 (*)
+| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| +--- com.android.volley:volley:1.1.1 -> 1.2.0
+| +--- androidx.paging:paging-runtime:2.1.2
+| | +--- androidx.paging:paging-common:2.1.2
+| | | +--- androidx.annotation:annotation:1.0.0 -> 1.7.0 (*)
+| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.6.2 (*)
+| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.6.2 (*)
+| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
+| +--- com.goterl:lazysodium-android:5.0.2
+| +--- net.java.dev.jna:jna:5.5.0
+| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.9.10 (*)
+| +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.20 -> 1.9.22
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.9.22 (*)
+| +--- androidx.appcompat:appcompat:1.0.2 -> 1.6.1 (*)
+| +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.2 (*)
+| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6
+| | \--- androidx.annotation:annotation:1.2.0 -> 1.7.0 (*)
+| +--- androidx.security:security-crypto:1.0.0 -> 1.1.0-alpha03
+| | +--- androidx.annotation:annotation:1.1.0 -> 1.7.0 (*)
+| | +--- com.google.crypto.tink:tink-android:1.5.0
+| | \--- androidx.collection:collection:1.1.0 -> 1.4.0 (*)
+| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0
+| | +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.9.10 (*)
+| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+| +--- org.apache.commons:commons-text:1.10.0 (*)
+| +--- androidx.room:room-runtime:2.4.2 -> 2.5.2 (*)
+| +--- androidx.room:room-ktx:2.4.2 -> 2.5.2
+| | +--- androidx.room:room-common:2.5.2 (*)
+| | +--- androidx.room:room-runtime:2.5.2 (*)
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
+| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.7.3 (*)
+| +--- com.google.dagger:dagger:2.42 -> 2.50
+| | \--- javax.inject:javax.inject:1
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.7.3 (*)
+| \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.7.3 (*)
-\--- org.wordpress.fluxc.plugins:woocommerce:trunk-cfe34975d20df76a2b7b4522dccd2faf53ef0f7c
- +--- org.wordpress:wellsql:2.0.0 (*)
- +--- org.wordpress.fluxc:fluxc-annotations:trunk-cfe34975d20df76a2b7b4522dccd2faf53ef0f7c
- +--- androidx.room:room-ktx:2.4.2 -> 2.5.2 (*)
- +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.9.10 (*)
- +--- org.wordpress:fluxc:trunk-cfe34975d20df76a2b7b4522dccd2faf53ef0f7c (*)
- +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
- +--- com.google.dagger:dagger:2.42 -> 2.50 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.7.3 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.7.3 (*)
- \--- androidx.room:room-runtime:2.4.2 -> 2.5.2 (*)
+\--- org.wordpress.fluxc.plugins:woocommerce:3008-f3ff9689a0a998537a624ce9a77b930a341bee48
+ +--- org.wordpress:wellsql:2.0.0 (*)
+ +--- org.wordpress.fluxc:fluxc-annotations:3008-f3ff9689a0a998537a624ce9a77b930a341bee48
+ +--- androidx.room:room-ktx:2.4.2 -> 2.5.2 (*)
+ +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.9.10 (*)
+ +--- org.wordpress:fluxc:3008-f3ff9689a0a998537a624ce9a77b930a341bee48 (*)
+ +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+ +--- com.google.dagger:dagger:2.42 -> 2.50 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.7.3 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.7.3 (*)
+ \--- androidx.room:room-runtime:2.4.2 -> 2.5.2 (*)
Please review and act accordingly
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
ddffdd6
to
146964a
Compare
@hichamboushaba done! Ready for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @0nko 👏, I left some comments, mostly nit picks, but the first one is important IMO.
@@ -52,6 +52,7 @@ data class Order( | |||
val selectedGiftCard: String?, | |||
val giftCardDiscountedAmount: BigDecimal?, | |||
val shippingTax: BigDecimal, | |||
val billingName: String = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some issues with this change to introduce billingName
as a read-only property:
- Adds to the size of the
Order
Parcelable, which is a minor thing, but something we should still avoid unless necessary. - Could lead to inconsistencies:
- we can update the
billingAddress
usingcopy
function, which I believe we do in the Order edit screen, so when this happen, thebillingName
will still point to an outdated value. - For someone unfamiliar with the code, they could think that updating the
billingName
is enough to update the customer's name, which is not the case.
- we can update the
As a solution, I suggest two solutions:
- Either use the existing function
Order#getBillingName
- Introduce a computed property
billingName
that returns an empty string instead of the default value we use now, and then supply the default value in the UI code, this would be something like:
@IgnoredOnParcel
val billingName
get() = getBillingName("").trim()
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks.
@@ -165,6 +166,8 @@ class DashboardFragment : | |||
|
|||
is FeedbackNegativeAction -> mainNavigationRouter?.showFeedbackSurvey() | |||
|
|||
is NavigateToOrders -> mainNavigationRouter?.showOrders() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np, seeing you introduce the function showOrders
here got me thinking on why we need it, and it turns out, we don't, the Navigation Component and the integration with the Bottom Navigation can handle this for us with a simple call to findNavController.navigate(R.id.orders)
, this means that if you want, you can handle this navigation event directly from the card in a HandleEvent
function, and decouple it from the parent ViewModel, but it's a minor thing, feel free to ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there might be an easier way but I didn't think of this, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed your advice but now the bottom navigation button don't work right. I pushed the code so you could take a look while I'm trying to figure out if I can make it work, or if I need to revert the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed your advice but now the bottom navigation button don't work right. I pushed the code so you could take a look while I'm trying to figure out if I can make it work, or if I need to revert the change.
Sorry I missed this case when I suggested this solution, I found a fix after checking the code of NavigationUI.onNavDestinationSelected
:
navController.navigateSafely(
resId = R.id.orders,
navOptions = navOptions {
popUpTo(navController.graph.findStartDestination().id) {
saveState = true
}
}
)
But I'm not sure if this is simpler than what you had before or not, I'll leave it to you to decide which solution is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you!
@@ -122,6 +126,38 @@ class OrderListRepository @Inject constructor( | |||
} | |||
} | |||
|
|||
fun observeTopOrders(count: Int, statusFilter: Order.Status? = null) = flow { | |||
emit(Result.success(emptyList())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np, do we need to start with an empty list? I think we can avoid the double emission by just removing the takeIf { it.isNotEmpty() }
below, WDYT?
@@ -88,10 +88,9 @@ class DashboardRepository @Inject constructor( | |||
isSelected = widget.isAdded, | |||
status = when (type) { | |||
DashboardWidget.Type.STATS, | |||
DashboardWidget.Type.ORDERS, | |||
DashboardWidget.Type.POPULAR_PRODUCTS -> statsWidgetsStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you used the hasOrders
logic to hide the card 👏, just one np point, I think it would be more clearer to rename the class and property now, saying statsWidgetStatus
is not very clear.
And also we should probably let the iOS team know about this decision (unless this was done and I missed it).
if (orders.isEmpty()) { | ||
ViewState.Loading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np, coupling the empty state to loading could break easily, Do you think we can avoid it with using transformLatest
? It's something I did in the reviews card and it worked well.
orders | ||
.toList() | ||
.takeIf { it.isNotEmpty() } | ||
?.let { emit(Result.success(it)) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this logic, we'll emit the cached value first even when the user pulls-to-refresh, WDYT about passing an argument isForced
and skip the cache when it's true
?
Thank you for the great feedback, @hichamboushaba! |
@hichamboushaba I think I've addressed all feedback, ready for another round! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/model/DashboardWidget.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/DashboardContainer.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/dashboard/data/DashboardDataStore.kt # WooCommerce/src/main/res/values/strings.xml
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #11486 +/- ##
============================================
- Coverage 40.64% 40.49% -0.16%
Complexity 5181 5181
============================================
Files 1073 1075 +2
Lines 62609 62844 +235
Branches 8565 8610 +45
============================================
+ Hits 25446 25447 +1
- Misses 34875 35109 +234
Partials 2288 2288 ☔ View full report in Codecov by Sentry. |
Implements #11461 and displays the Most recent orders dashboard card (without the filtering header).
To test:
Most recent orders
card is shownView all orders